Skip to content

v1.1#3

Open
royalgarter wants to merge 2 commits intoprofessorhantzen:masterfrom
royalgarter:master
Open

v1.1#3
royalgarter wants to merge 2 commits intoprofessorhantzen:masterfrom
royalgarter:master

Conversation

@royalgarter
Copy link

  • Add cluster for multi core CPU usage
  • Export result to file
  • Support regex vanity string
  • Default max attemps = Math.round(Number.MAX_VALUE/2);
  • Some minor cleanup

* Add cluster for multi core CPU usage
* Export result to file
* Support regex vanity string
* Default max attemps = Math.round(Number.MAX_VALUE/2);
* Some minor cleanup
@royalgarter
Copy link
Author

royalgarter commented Sep 20, 2017

Sorry for some overkill cleanup that I have made. At first I didn't intent to create pull request (just fork for myself) but then I think that could benefit other users so ...

Btw, I see on NPM we already have vanity-key (BTC) and vanity-eth (ETH), so may I suggest that we could rename package to vanity-xrp/vanity-ripple before submit to NPM for ease of search?

@professorhantzen
Copy link
Owner

professorhantzen commented Sep 21, 2017

Hi there! I appreciate the work and your willingness to share it this way - though I will understand if you want to reconsider. I have intended to add cluster but haven't had time - unfortunately doing this requires asynchronous message handling between the forks in order to work in a way I'd be comfortable with. At the moment, unfortunately your version is quite broken (at least on my machine).

If it helps, here are the issues I've identified:

  1. If started without a string, it prints "Vanity is valid: undefined", and then the usage twice before throwing an error. A common first run for a new user is to start a script with no options as the user may not initially know what the options are. Traditionally, command line programs display usage instructions when started without parameters and that's the behaviour I'd intended. At the moment it's unclean and the error creates a first impression of being broken (which will result in user questions/issues).
  2. If started with a string, it prints "Vanity is valid: true" num_cores times, prints the full usage and the string with slashes around it and a trailing "i" (at least on my system). In this case it shouldn't print the usage is it's being used correctly, only the search status need be printed. The search string should be printed clearly as this is an important piece of feedback to the user - they may feel uneasy if they see a modified string displayed back to them prior to committing to a long search taking up all cores of their machine. Also, the validity of the string should only be checked once on master - not by every slave core. This also needn't be asserted to the user - the intended behaviour is if the script prints the usage and does not start searching, it can be assumed by the user they've done something wrong (such as supplied an incorrect string which is about the only thing they can do wrong in the case of your modified version).
  3. The search status % is erratic and jumps backward and forward, as the cores get out of sync when processing the for loop.
  4. When ending it always outputs "0" addresses found, regardless of how many were in fact found.
  5. Because of the way "append" is used, on some file systems (at least on mine), the file is never created because it doesn't first exist in order to be "appended" to. In other words, at the moment if I run your script correctly, I always get no addresses I can use.
  6. Not everyone knows what regex is or how to use it but the usage instructions make it seem required knowledge and some users will assume it is not possible to search for a normal string, when in fact it is.
  7. The amount specifier is presently multiplied by the number of cores, but this is done silently and without the knowledge of the user, and the interface creates the impression it's searching through the user-supplied amount of addresses, when in fact it's searching for that amount * num_cores.
  8. Rather than using MAX_VALUE, which clutters the output with a long number and exponent, it might be better to allow the user to specify a search iterator of "0" to allow the script to run indefinitely (as that is presumably the intention). In such a case of running indefinitely the progress bar could be eliminated.

@royalgarter
Copy link
Author

royalgarter commented Oct 2, 2017

Thanks for your response & sorry for my naive. I see most of your points are valid. Let's see if I could find a way to solve these problems and re submit it again.

P/S: Most of the points is around the "cluster" problem. So let me dig deeper in this and make it work through first.

@professorhantzen
Copy link
Owner

No problem! Apologies for the late reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants